-
-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pkgs.callOverlay function #180557
Add pkgs.callOverlay function #180557
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
2319ded
to
4f59340
Compare
This will fail with |
Ahh, there it is. And here I was thinking that it was too simple. Will fix it later today/tomorrow |
4f59340
to
86d9e11
Compare
86d9e11
to
10e0ac7
Compare
Ready to be merged |
I am quite happy with the result. Big thanks to @zimbatm for pointing out that |
Co-authored-by: Artturi <Artturin@artturin.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too coupled to stage.nix
. It would benefit from being implemented there instead.
With a more "higher order function" flavored solution in stage.nix
, we can make this coherent instead of coupled.
pkgs = newPkgs; | ||
callPackage = pkgs.lib.callPackageWith newPkgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the other package sets like pkgsStatic
or the cross ones?
As a general rule, overriding can not be added externally like you're attempting here. It needs to be integrated in the original structure because of all the possible recursive definitions. In other words, this needs changes in top-level/stage.nix
in order to work well.
We haven't even made pkg.override
work entirely correctly with cross splicing and overrideAttrs
yet, and this change has a lot in common with a new "pkg.override".
For this functionality to not be another series of bugs like we've had with overriding, we need better support for sub-packagesets in stage.nix
.
One idea is to make its nixpkgsFun
overridable, although that might re-evaluate too much. Another is to put all the extra package sets in their own overlay, so that not just pkgs
but all the package sets are easily modifiable.
Here's the general idea. It probably contains some mistakes, but it conveys the idea better than English.
let
# adaptPackageSet :: attrs -> attrs -> attrs
# adaptPackageSet = pkgs: definition: ...
# Internal to stage.nix.
# A function that defines how the package set is extended.
# - `pkgs` an already evaluated (or at least shared) instance of the Nixpkgs fixpoint.
# - `definition` a fresh redefinition of the package set according to the rules of e.g. `pkgsLLVM`, `pkgsCross.<x>`, etc.
otherPackageSets = adaptPackageSet: self: super: {
pkgsLLVM = adaptPackageSet super.pkgsLLVM (
# the existing definition
nixpkgsFun { /* ... */ }
);
# pkgsCross etc
# pkgs?
};
callOverlayOverlay = otherPackageSets (super: definition: callOverlay' super);
callOverlay' = overlay: callOverlay (/* flip ? */ extends callOverlayOverlay overlay);
callOverlay = your definition;
in
# toFix = ... [
# ...
(otherPackageSets (super: definition: definition))
# ...
(_: _: { callOverlay = callOverlay'; }) # though not sure if this should be separate (and add a somewhat expensive // on pkgs)
# ...
extend
should probably be defined similarly, but ignore the super
argument of wrapPackageSet
.
I think there's also a possibility of allowing actual overlays to extend the otherPackageSets
function, but I'm not sure and that's over-engineering it for the first design step, but it would fix another problem. First priority would be to make something like the above work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB this still doesn't cover interactions between extend
and callOverlay
.
Also how about affixOverlay
or attachOverlay
? They're not properly merged into self like an overlay normally would, so I think it would be good to add an appropriate verb. extend
already doesn't really suggest merging, so I tried to think of something that is even less "merge"-like.
EDIT: add "into self"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing due to amount of edge cases and there is no clean way to cover them |
Description of changes
Added
callOverlay
function. This function helps us evaluate overlays without applying them to nixpkgs. Similar idea toextend
function but does get polluted with a global context.Boilerplate for reviewers to test this:
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes